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“The competent programmer is fully aware of the strictly 
limited size of his own skull; therefore he approaches 
the programming task in full humility, and among other 


things he avoids clever tricks like the plague. 
— Edsger Dijkstra © 2021 Philip Koopman 1 
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Peer Reviews anels 


University 


= Anti-Patterns: 
e No peer reviews 
e Reviews too informal/too fast 
e Reviews find <50% of all bugs 


= Fresh eyes find defects 
e Code and other document benefit 
from a second (and third) set of eyes 
e Peer reviews find more bugs/S than testing 
— And, they find them earlier when bugs are cheaper to fix 
e Everything written down can benefit from a review 
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Most Effective Quality Practices Mellon 


University 


Ebert & Jones, “Embedded Software: Facts, Figures, and Future,” 1EEE Computer, April 2009, pp. 42-52 
Ranked by defect removal effectiveness in percent defects detectable at that stage that are removed. 
“k” means exceptionally productive technique (more than 750+ function points/month) 


* 87% static code analysis (“lint” tools, compiler warnings) 
85% design inspection 

85% code inspection 

82% Quality Function Deployment (requirements analysis) 
80% test plan inspection 

78% test script inspection 

* 77% document review (other documents) 

75% pair programming (informal on-the-fly review) 

70% bug repair inspection 

* 65% usability testing 

50% subroutine testing (unit test) 

* 45% SQA (Software Quality Assurance) review 

@ 40% acceptance testing © 2021 Philip Koopman 3 
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Peer Reviews Are Effective + Efficient Mellon 
| University 
Defect Removal by Phase - Typical Project from 5 years earlier 
mo . . Most bugs found 
2 hee No reviews, no unit test, in system test! 4 Minor 
3 no integration test. ... ‘Ny = Major 
50 
System Software Arch Det Design Code Unit Test Integ Test System 
Rqmts Rqmts Design Test 
9 years later... Defect Removal by Phase With Peer Reviews Almost no bugs left 
a Found more bugs total in system test! 
D 200 
r= 150 C1 Minor 
5 100 i Major 
50 
0 System Software Arch Det Design Code Unit Test Integ Test System 
Reaots Rqmts Design Test [Source: 
Roger G., 
Aug. 2005] 


Found many bugs up front, where fixes are cheaper 
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Gold Standard: Fagan Style Inspections Caiveaity 


= Methodical, in-person review meetings 
Pre-meeting familiarity with project 
Producer explains item then leaves 
Moderator keeps things moving 

Reader (not author) summarizes as you go 
Reviewers go over every line, using checklists (perspective-based) 
Recorder takes written notes 

Result: written list of defects. The Producer fixes code off-line 
Re-inspection if the defect rate was too high 





= Methodical reviews are the most cost effective 
e Important to measure bug discovery rate to ensure review quality 
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Rules for Successful Peer Reviews peu 


Inspect the item, not the author 

e Dont attack the author. 

Dont get defensive 

e Nobody writes perfect code. Get over it. 
Find but don't fix problems 

e Donttry to fix them; just identify them. 
Limit meetings to two hours 

e People are less productive after that point. 
Keep a reasonable pace 

e About 150 lines of code (or equivalent) per hour. Too fast and too slow are both bad. 
Avoid “religious” debates on style 

e Enforce conformance to your style guide. No debates on whether style guide is correct. 
Inspect, early, often, and as formally as you can 

e Keep records to document value (might take a while to mature). 











ert) ae 


No Playing& Jumping No Slapping 
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Example Light-Weight Review Report Mellon 
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Date: 
Artifact: 
Reviewers: 
Size: 


Time Spent: 


# Issues: 
Outcome: 


Issue# 
1 


Co “So on fs oo fo 


Status Key: 


Peer Review Template for Project X 
4/17/2011 
Ayzzy.cpp Functions: Foo(), Bar(), Baz() 
Stella K., Joe B.. Sam Q., Trish R. 


oe # issues found is the most astra Just ner 
3 6 important et | fixed if fixed 
Re-Review of Bug Fixes Required + ah 

24 hours 





Issue Description statu’, A 
Issue 7.. Fixed 


ay Free form text issue ae 
Issue 3.. “£ description Bugzilla 


Issue 4.... Not a Bug 





Fixed (trivial fix by author: no need to enter in defect list) 
Bugzilla (entered into project defect system) 
Not a Bug (false alarm) 
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= Perspective-based Peer Reviews are 35% more effective 


[https://www.cs.umd.edu/projects/SoftEng/ESEG/papers/82.78.pdf] 
= Mechanics of a Perspective-based review 
e Divide a peer review checklist into three sections 
e Assign each participant a different section of the checklist 
— OK to notice other things, but primary responsibility is that section 
— Multiple sets of eyes + perspective breadth 


= Example perspectives for a review: 
e Control flow issues 
e Data handling issues 
e Style issues 
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Peer Review Checklist Template Mellon 


University 





Before Review: 


Peer Review Checklist: Embedded C Code 


= Customize 
as needed 


0 _ Code compiles clean with extensive warning checks (e.g. MISRA C rules) 
Reviewer #1: 
1 Commi Reviewer #2: Reviewer #3: 
2 __ Stylec, 8 __ Single point 4, _____ Minimum scope for all functions and variables; essentially no globals 
3 ___ Proper 9 ___ Loop entryé ,, —___ Concurrency (locking, volatile keyword, minimize blocking time) 
4 ___ Noorg 10 ____ Conditionals ;. Input parameter checking (style, completeness) 
5 ____ Condit 11 ____All functions 19 ____ Error handling for function returns 
6 _ Parent 12 ___ Use const ar 5, _____ Handle null pointers, division by zero, null strings, boundary conditions 
7 ____ Allswi 13 ___ Avoid use of 21 __ Floating point issues (equality, NaN, INF, roundoff); use of fixed point 
_ 14 ____ Strong typin 5, ____ Buffer overflow safety (bound checking, avoid unsafe string operations) 
15 __ All variables 


All Reviewers 
23 _—«s*Does the code match the detailed design (correct functionality)? 
24 ___ Is the code as simple, obvious, and easy to review as possible? 
For [WO Reviewers assign items: Reviewer#1: 1-11; 23-74 Reviewer#2: 12-74 
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Before (Ineffective Reviews) Mell 





<> Median 
— Mean 


= — — —12 Hrs/Wk 








Average Median = 12.9 
Average Mean = 13.4 





=) 


Weekly Hours 


> 
> 
* > 
. — = 


10 
VWeeks Finals 
Summed Weak 
(Zero hrs) 
0 > 
0 2 4 6 S 10 12 14 16 
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Weekly Hours 


With Weekly Defect Reporting 


30.0 
20.0 
10.0 
0.0 
O = 4 6 8 10 
Week # 


12 


14 


Carnegie 
Mellon 
University 





16 
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_ Review More Than Just The Code Melon 


gle cea ee, sastevaretessesases seeps 
x Testing 


Requirements 



























Integration 
Testing 











Architecture & HLD 

















LEGEND: 


Artifacts 
To Peer 
Review 





“+ Test Plan a. om Subsystem 
Testing 
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_ Economics Of Peer Review Sate 


University 





m Peer reviews provide more eyeballs to find bugs in an affordable way 
e Good embedded coding rate is 1-2 lines of code/person-hr 
— (Across entire project, including reqts, test, etc.) 
e A person can review 50-100 times faster than they can write code 
— If you have 4 people reviewing, that is still >10x faster than writing! 
e How much does peer review cost? 
— 4 people * 100-200 lines of code reviewed per hour 
— E.g., 300 lines; 4 people; 2 hrs review+1 hr prep = 25 LOC/person-hr 
e Reviews are only about 5%-10% of your project cost 
= Good peer reviews find at least half the bugs! 


e And they find them early, so total project cost can be reduced 





m= Why is it folks say they don't have time to do peer reviews? 
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Peer Review Best Practices fee 


University 





= Formal reviews (inspections) optimize bugs/$ 
e Target 10% of project effort to find 50% of bugs 
— You can review 100x faster than write code; it's cheap 
e Review everything written down, not just code 
e Use a perspective-based checklist to find more bugs 
= Review pitfalls 
e If your reviews find <50% of defects, they are BROKEN 
— The 80/20 rule does NOT apply to review formality! Formal reviews are best. 
— You cant review at end; need to review throughout project 
= Review tools 
e On-line review tools are OK, but not a substitute for in-person meeting 
e Static analysis tools are great — but not a review! 
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YOUR CODE. LOOKS LIKE 
SONG LYRICS WRITTEN 
USING ONLY THE STUFF 
THAT COMES AFTER THE 


‘IT'S LIKE A JSON 


TABLE OF MODEL 
NUMBERS FOR 
FLASHLIGHTS 
WITH “TACTICAL 





LIKE YOU READ TURING'S 
956 PAPER ON COMPUTING 
AND A PAGE OF JAVASCRIPT 
AT EVERYTHING IN BETWEEN. 





IT'S LIKE A LEET-SPEAK TRANSLATION 
OF A MANIFESTO BY A SURVIVALIST CULT 
LEADER WHOS FOR SOME KEASC 

OBSESSED WITH MEMORY ALLOCATION. 


| I CAN GET SOMEONE 


NOT MORE THAN 
ONCE, L GET. 





https://www.xkcd.com/1833/ 
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